Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decrease the default number of rings in CylinderMesh to 0 #49730

Closed
wants to merge 1 commit into from
Closed

Decrease the default number of rings in CylinderMesh to 0 #49730

wants to merge 1 commit into from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 19, 2021 •
edited
Loading

Follow-up to #49729.

Subdividing a CylinderMesh is only needed when deforming it in a shader. The new default rings value decreases the triangle count for a CylinderMesh by a factor of 5.

The property hint was also tweaked to allow setting the number of rings to 0, which is a valid value.

Preview

Before

Subdivided cylinder

After

Non-subdivided cylinder

@Calinou Calinou requested a review from a team as a code owner June 19, 2021 06:50
@Calinou Calinou added this to the 4.0 milestone Jun 19, 2021
@Calinou Calinou mentioned this pull request Jun 19, 2021
Merged
@akien-mga
Copy link
Member

I'm not really competent in that field so just asking to have it discussed: can it cause issues with some algorithms to have such thin elongated tris in the 0 rings version?

@Calinou
Decrease the default number of rings in CylinderMesh to 0
f31e103 
Subdividing a CylinderMesh is only needed when deforming it in a shader.
The new default rings value decreases the triangle count for a
CylinderMesh by a factor of 5.

The property hint was also tweaked to allow setting the number of
rings to 0, which is a valid value.
@Calinou Calinou force-pushed the cylindermesh-decrease-rings-default branch from c5569f5 to f31e103 Compare August 16, 2021 04:01
@reduz
Copy link
Member

reduz commented Aug 24, 2021

I think @BastiaanOlij should give a check to this

@clayjohn
Copy link
Member

I don't think this change is an improvement.

  • The difference in vertex count is negligible (GPUs are made to render thousands at a time, and there is no speed benefit unless you are reducing vertex count by many thousand). The only platform where the reduced vertex count may be beneficial is mobile, but this is bad for mobile for other reasons (below).

  • Long thin triangles can lead to worse performance (especially on mobile, but even on desktop)

  • Long thin triangles will look horrible with vertex lighting

@Calinou Calinou closed this Aug 25, 2021
@Calinou Calinou mentioned this pull request Sep 1, 2021
Open
@Calinou Calinou mentioned this pull request Oct 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pouleyKetchoupp pouleyKetchoupp Awaiting requested review from pouleyKetchoupp

Assignees
No one assigned
Labels
archived breaks compat enhancement topic:core topic:3d
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Calinou @akien-mga @reduz @clayjohn